Skip to content

feat: Add CSV output format for default list tools under insiders mode#2450

Merged
SamMorrowDrums merged 11 commits into
mainfrom
rosstarrant/add-csv-output-structure
May 21, 2026
Merged

feat: Add CSV output format for default list tools under insiders mode#2450
SamMorrowDrums merged 11 commits into
mainfrom
rosstarrant/add-csv-output-structure

Conversation

@RossTarrant
Copy link
Copy Markdown
Contributor

@RossTarrant RossTarrant commented May 11, 2026

Summary

Adds feature-gated CSV output for default list_* tools and refactors Insiders mode into a feature flag bundle.

Why

CSV provides a flatter, more compact representation for list-style responses, helping evaluate token reduction from output format changes.

Refs github/copilot-mcp-core#1323

What changed

  • Added csv_output as an allowed Insiders feature flag.
  • Added feature-gated JSON/CSV variants for list_* tools.
  • Added central JSON text response to CSV conversion with dot-notation flattening for nested objects.
  • Normalised whitespace in body fields for CSV output.
  • Refactored Insiders mode so it expands to curated feature flags instead of being checked directly by tools.
  • Added ifc_labels as an Insiders-enabled feature flag.
  • Updated MCP Apps and IFC gates to check concrete feature flags.
  • Added unit coverage for CSV conversion, feature-gated variants, and Insiders feature expansion.

MCP impact

  • No tool or API changes
  • Tool schema or behavior changed
    CSV mode changes list_* tool (within default toolset) response behavior under the csv_output Insiders feature flag. Tool schemas are unchanged; CSV output is server-controlled by feature flag gating.
  • New tool added

Prompts tested (tool changes only)

  • "List discussions in RossTarrant/octopus-mcp"
  • "List issues in github/github-mcp-server"
  • "List pull requests in github/github-mcp-server"

Security / limits

  • No security or limits impact
  • Auth / permissions considered
  • Data exposure, filtering, or token/size limits considered
    CSV mode is intended to reduce response size for list tools. Body fields are whitespace-normalized in CSV output; full formatted content remains available through corresponding get/read tools.

Tool renaming

  • I am renaming tools as part of this PR (e.g. a part of a consolidation effort)
  • I have added the new tool aliases in deprecated_tool_aliases.go
  • I am not renaming tools as part of this PR

Lint & tests

  • Linted locally with ./script/lint
  • Tested locally with ./script/test

Docs

  • Not needed
  • Updated (README / docs / examples) - Updated docs/insiders-features.md detailing the new experimental feature

@RossTarrant RossTarrant force-pushed the rosstarrant/add-csv-output-structure branch 2 times, most recently from 8e2be4a to 3ada757 Compare May 12, 2026 07:44
@RossTarrant RossTarrant marked this pull request as ready for review May 12, 2026 07:49
@RossTarrant RossTarrant requested a review from a team as a code owner May 12, 2026 07:49
Copilot AI review requested due to automatic review settings May 12, 2026 07:49
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds an Insiders-gated CSV output mode for default list_* tools, aiming to reduce response size/context for MCP clients. This is implemented by feature-flagged tool variants that convert the existing JSON text output into a flattened CSV representation.

Changes:

  • Added csv_output feature flag (allowlisted and enabled by Insiders mode).
  • Introduced feature-gated JSON/CSV tool variants for default-toolset list_* tools, with centralized JSON→CSV conversion (flattening + whitespace normalization).
  • Wired HTTP mode CLI --features / --insiders into feature-flag resolution; added unit tests + Insiders feature documentation.
Show a summary per file
File Description
cmd/github-mcp-server/main.go Passes CLI --features into HTTP server config.
pkg/http/server.go Extends HTTP server config with static enabled features; updates feature checker to combine static + header features and insiders mode.
pkg/http/server_test.go Adds coverage for static feature/static insiders behavior in HTTP feature checker.
pkg/http/handler_test.go Updates tests to new feature-checker signature.
pkg/github/feature_flags.go Adds csv_output flag to allowlist + insiders expansion list.
pkg/github/tools.go Wraps tool list construction to inject CSV-output variants for eligible tools.
pkg/github/csv_output.go Implements JSON-text tool-result → flattened CSV conversion and tool handler wrapping.
pkg/github/csv_output_test.go Adds unit tests for CSV conversion + feature-gated tool variants.
docs/insiders-features.md Documents CSV output behavior, format, and how to enable it.

Copilot's findings

  • Files reviewed: 9/9 changed files
  • Comments generated: 1

Comment thread pkg/github/csv_output.go
@RossTarrant RossTarrant force-pushed the rosstarrant/add-csv-output-structure branch from 3ada757 to b070b00 Compare May 12, 2026 08:00
@RossTarrant RossTarrant self-assigned this May 18, 2026
Copy link
Copy Markdown
Collaborator

@SamMorrowDrums SamMorrowDrums left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RossTarrant I thought in a previous version similar bit of work I had asked that insiders would be expanded to the feature flags it enables, and nothing would be gated on insiders, but the feature that it is actually related to, and insiders is just a shortcut to enabled flags. Just like all and default are meta toolsets (contain more than one toolset), insiders should just be a meta feature flag, that enables a set of feature flags, so at the point they are checked, they should be considered to be regular feature flags.

Nothing else in the codebase should need to know what insiders is. That just leaks the concept too far.

Can this PR resolve that while it's making changes to the feature checker?

Comment thread internal/ghmcp/server.go Outdated
Comment on lines +349 to +352
featureSet := github.ResolveFeatureFlags(
enabledFeatures,
github.MetaFeatureFlagsForInsiders(insidersMode)...,
)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need to resolved the user provided enabled feature flags by the allowed features:

func ResolveFeatureFlags(enabledFeatures []string, insidersMode bool) map[string]bool {

Because insiders mode flags list should be already in there. Also can't this directly reference the list of insiders features?

var InsidersFeatureFlags = []string{

Comment thread internal/ghmcp/server.go
Comment on lines -375 to -377
if cfg.InsidersMode {
userAgent += " (insiders)"
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RossTarrant I think we do still want the UA set here. Maybe there is minor reason for some places to know insiders is on, albeit minimal. Apologies.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SamMorrowDrums I took your previous comment a bit too far! 😄 Have added it back

RossTarrant and others added 2 commits May 21, 2026 09:05
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@RossTarrant RossTarrant force-pushed the rosstarrant/add-csv-output-structure branch from 1491adb to bfa6fee Compare May 21, 2026 08:26
Comment thread pkg/http/middleware/cors.go Outdated
"Mcp-Session-Id",
"Mcp-Protocol-Version",
"Last-Event-ID",
"X-Custom-Auth-Headers",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this needed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could make it a separate PR if needed, but this is required to allow X-MCP-FEATURES headers

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I've switched it for the correct header in 0c9b8b3

headers.MCPExcludeToolsHeader,
headers.MCPFeaturesHeader,
headers.MCPLockdownHeader,
headers.MCPInsidersHeader,
Copy link
Copy Markdown
Collaborator

@SamMorrowDrums SamMorrowDrums May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be missing the features header actually. So end user can enable via HTTP.

You can check remote, but I believe it is x-mcp-features or similar.

Comment thread pkg/inventory/filters.go Outdated

// AvailableToolsWithoutFeatureFiltering returns tools that pass every filter
// except FeatureFlagEnable/FeatureFlagDisable.
func (r *Inventory) AvailableToolsWithoutFeatureFiltering(ctx context.Context) []ServerTool {
Copy link
Copy Markdown
Collaborator

@SamMorrowDrums SamMorrowDrums May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am curious why we need this? Is it just for testing? I would have thought it would be sufficient to just either have features on or of. The disable tools are always on if no features are present.

Copy link
Copy Markdown
Collaborator

@SamMorrowDrums SamMorrowDrums left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is getting there, my biggest concern is that the way CSV tools is being added, it doesn't look like it will correctly have the http mode enable/disable, and curious how that would working in github/github-mcp-server-remote also whether the wiring would automatically work or not.

Comment thread pkg/inventory/filters.go Outdated
})
}

sortTools(result)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we don't ultimately need this function than maybe we don't need to extract sort.

Comment thread pkg/inventory/filters.go Outdated
Comment on lines +182 to +187
func (r *Inventory) AvailableResourceTemplatesWithoutFeatureFiltering(_ context.Context) []ServerResourceTemplate {
var result []ServerResourceTemplate
for i := range r.resourceTemplates {
res := &r.resourceTemplates[i]
if r.isToolsetEnabled(res.Toolset.ID) {
result = append(result, *res)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same patter, but I'm not sure why we need.

Comment thread pkg/inventory/filters.go Outdated
Comment on lines +227 to +238
// AvailablePromptsWithoutFeatureFiltering returns prompts that pass every filter
// except FeatureFlagEnable/FeatureFlagDisable.
func (r *Inventory) AvailablePromptsWithoutFeatureFiltering(_ context.Context) []ServerPrompt {
var result []ServerPrompt
for i := range r.prompts {
prompt := &r.prompts[i]
if r.isToolsetEnabled(prompt.Toolset.ID) {
result = append(result, *prompt)
}
return result[i].Prompt.Name < result[j].Prompt.Name
})
}

sortPrompts(result)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question here. Also, you can use Go generics to not need to do a separate typed sort function per type.

SamMorrowDrums and others added 7 commits May 21, 2026 15:51
…ering when no checker (#2516)

* refactor: generic toolset+name sort, clarify feature flag intent

Address review feedback on #2450:

- Collapse the three near-identical sort helpers in pkg/inventory/filters.go
  into a generic sortByToolsetThenName so adding new inventory item types
  doesn't require copying the comparator.
- Expand the doc comments on the three *WithoutFeatureFiltering helpers to
  spell out why they exist: HTTP mode builds a static (process-wide)
  inventory as an upper bound, but per-request feature flags from headers
  (X-MCP-Features, X-MCP-Insiders) are evaluated later, so feature-flagged
  variants must be preserved here.
- Strengthen the doc comment on ResolveFeatureFlags to make the contract
  explicit: user-supplied flags are validated against AllowedFeatureFlags,
  but insiders expansion deliberately is not — InsidersFeatureFlags may
  include server-controlled flags that are not user-toggleable.

CORS comments are intentionally left for the PR author.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* docs(feature-flags): clarify allowed and insiders sets are independent

Also add tests covering:
- a user-toggleable flag (FeatureFlagIssuesGranular) that insiders does
  not turn on automatically
- insiders mode not turning on user-only allowed flags

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* refactor(inventory): collapse three *WithoutFeatureFiltering helpers into StaticUpperBound

The three parallel methods (AvailableToolsWithoutFeatureFiltering,
AvailableResourceTemplatesWithoutFeatureFiltering,
AvailablePromptsWithoutFeatureFiltering) were always called as a triple
in exactly two places: HTTP buildStaticInventory and its test mirror.
They exist because the dual-variant pattern (sibling tools with mirrored
FeatureFlagEnable / FeatureFlagDisable on the same name, e.g. CSV output)
makes feature filtering at static-build time impossible — both variants
must be kept and resolved per-request.

Replace the three with one method, Inventory.StaticUpperBound(ctx), that
returns (tools, resources, prompts) and carries the rationale in its
doc comment. Reduces API surface, eliminates the triplication, and makes
the single "skip feature filtering" concept obvious to readers.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* refactor: simplify feature-flag handling

Two related simplifications, both about treating insiders as a meta flag
that expands once at startup and then stops mattering:

- Collapse CSV's dual-variant pattern into a single tool whose handler
  performs a runtime feature-flag check via deps.IsFeatureEnabled. CSV
  is a pure response-format toggle, not a schema change, so it does not
  need the dual-name pattern that genuine schema variants (granular
  issues/PRs) still use.

- When no feature checker is installed, skip feature-flag filtering and
  return the full upper bound. The static HTTP inventory now uses plain
  AvailableTools/Resources/Prompts; the per-request inventory always
  installs a checker, so MCP registration (which serves a tool name once)
  always sees a deduplicated set. The bespoke StaticUpperBound helper and
  the isToolEnabledWithFeatureFlags split go away.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* ci(mcp-diff): add insiders + per-feature configs

The mcp-diff matrix now includes:
  - --insiders (and --insiders --read-only)
  - one config per github.AllowedFeatureFlags entry, generated by
    script/print-mcp-diff-configs so new user-controllable flags get
    diffed automatically without editing the workflow

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* docs(insiders): explain feature-flag resolution for contributors

Adds a 'How feature flags are resolved' section covering:
  - Insiders is a meta flag, like 'all'/'default' for toolsets
  - User input -> allowlist filter -> insiders expansion ->
    server-side fallback (remote only)
  - AllowedFeatureFlags vs InsidersFeatureFlags are independent
  - How to add a new feature flag, including the
    TestGitHubPackageDoesNotReadInsidersMode guard

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* refactor(inventory): make feature-flag gating a regular ToolFilter

Move tool feature-flag evaluation out of isToolEnabled and into a
ToolFilter installed at the head of the pipeline by Build() when
WithFeatureChecker received a non-nil checker. The 'no checker = no
filtering' contract is now expressed structurally (the filter isn't
installed) instead of by a runtime nil check inside the helper.

Resources and prompts have no filter pipeline, so they call the now-pure
featureFlagAllowed helper behind an explicit r.featureChecker != nil
guard at the iteration site.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* perf(inventory): cache extracted toolset IDs in sort comparator

Avoid evaluating the extractor closures up to three times per comparison.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…hub/github-mcp-server into rosstarrant/add-csv-output-structure
Adds a sibling mcp-diff-http job that exercises the streamable-http
transport against a shared HTTP server, with per-config settings supplied
via X-MCP-* request headers — mirroring how the remote server is invoked
in production (server-side defaults + per-user header overrides).

The config generator gains a -transport flag:
- stdio (default, unchanged behaviour)
- http-headers (emits headers-only configs targeting a shared server)

Two new combined entries layer multiple headers together as a smoke test
for header-merging regressions.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
SamMorrowDrums and others added 2 commits May 21, 2026 16:44
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@SamMorrowDrums SamMorrowDrums merged commit f929c58 into main May 21, 2026
19 checks passed
@SamMorrowDrums SamMorrowDrums deleted the rosstarrant/add-csv-output-structure branch May 21, 2026 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants